-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate Two Factor Authentication to use separate screens (remove react-native-animatable) #54404
Migrate Two Factor Authentication to use separate screens (remove react-native-animatable) #54404
Conversation
…ated for fade-in animations
…eaner implementation
0aaa367
to
2984009
Compare
react-native-animatable
with react-native-reanimated
react-native-reanimated
, deprecate react-native-animatable
, remove redundant useNativeDriver
Screen.Recording.2025-02-17.at.7.19.07.PM.mov@blazejkustra Can you add some guards to the pages? Looks like I can access any page without any problems directly through URL. |
Would not found page like below work for this? cc @shawnborton @dannymcclain 'Go back to security page' could be used instead of 'Go back to home page' ![]() |
That works for me, yeah! |
@blazejkustra let us know when this is ready for @shubham1206agra to take another look |
Ofc, I was checking one last thing 👍 |
@shubham1206agra Ready for another round of review! Some pages are still available, but functionality-wise it shouldn't affect the user: Screen.Recording.2025-02-18.at.16.24.12.mov |
@shubham1206agra what is your eta? |
Screen.Recording.2025-02-19.at.8.35.56.PM.movJust stating for the record. The screen transition is still stuttering. |
@shubham1206agra Turns out it only happens on some android emulators, more info here |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-02-19.at.9.05.05.PM.movAndroid: mWeb ChromeScreen.Recording.2025-02-19.at.8.37.12.PM.moviOS: NativeScreen.Recording.2025-02-19.at.8.54.51.PM.moviOS: mWeb SafariScreen.Recording.2025-02-19.at.8.25.49.PM.movMacOS: Chrome / SafariScreen.Recording.2025-02-19.at.8.08.25.PM.movMacOS: DesktopScreen.Recording.2025-02-19.at.8.44.02.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blazejkustra great job, just couple questions
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.3-0 🚀
|
Explanation of Change
Initially I wanted to migrate animatable to reanimated in one PR but changes from AnimatedStep component become too big to migrate everything in one PR. Here is the first PR.
The entire two-factor authentication (2FA) flow is designed to work within a single screen, which meant the animation had to closely mimic how react-navigation handles transitions.
This is the main issue with current transition, it doesn't align with how other screens in the app behave. Instead of starting from a consistent position, the animation began from an arbitrary point in the middle of the screen. This resulted in a clunky animation experience. Please check what is the difference on three platforms:
Before:
web-before.mov
ios-before.mp4
android-before.webm
After:
web-after.mov
ios-after.mp4
android-after.webm
Fixed Issues
$ #52920
PROPOSAL: #52920 (comment)
Tests
Video
copilot.mov
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Check
Explanation of Change
section with videos of how it looks now.